Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor certain doctests towards either reStructuredText documentation or Python tests #464

Merged
merged 24 commits into from
Dec 21, 2022

Conversation

amotl
Copy link
Member

@amotl amotl commented Oct 13, 2022

About

This is a stab at #448, to improve the shape of doctests vs. rendered documentation. It moves some doctest files around into the documentation directory, and dissolves others into pure-Python tests.

New documentation section

The doctest files referenced below are now becoming first citizens of the public documentation, within a dedicated "by example" section. To make sure they are always valid, they are still evaluated as doctests. Many thanks to the authors who originally conceived them.

After adding some introductory paragraphs to the individual files, I think the new content is sweet already, and will be even better after another iteration of more content-related edits and refactorings by a subsequent patch.

Here, the new documentation section is rendered as a preview on RTD, see CrateDB Python client by example.

Details

  • doctests/{client,http,blob}.txt and doctests/{connection,cursor,https}.txt have been moved to the documentation, to be rendered as reStructuredText (10da69c, 1731cc5).
  • All SQLAlchemy-related doctests are now also part of the documentation, that is doctests/sqlalchemy.rst and sqlalchemy/doctests/{dialect,itests,reflection}.txt (13ed96f), which are now located at docs/by-example/sqlalchemy/{cru,getting-started,inspection-reflection}.rst.
  • On the other hand, doctests/layer.txt and doctests/mocking.txt are now implemented as regular Python tests.
  • Any kinds of symbol aliases have been removed from the doctest launcher, established per test.globs, in order not to obfuscate them away from the users reading the examples, who are likely aiming to just copy/paste the presented code snippets.

@amotl amotl changed the base branch from master to amo/timezone-aware-datetimes October 13, 2022 13:28
@amotl amotl changed the title [WIP] Refactor certain doctests towards both documentation and Python tests [WIP] Refactor certain doctests towards either reStructuredText documentation or Python tests Oct 13, 2022
@amotl amotl force-pushed the amo/doctests-refactoring branch 2 times, most recently from 4ad6b09 to 13ed96f Compare October 13, 2022 18:26
@amotl amotl changed the title [WIP] Refactor certain doctests towards either reStructuredText documentation or Python tests [STACK] Refactor certain doctests towards either reStructuredText documentation or Python tests Oct 13, 2022
@amotl amotl force-pushed the amo/timezone-aware-datetimes branch 5 times, most recently from 135caf7 to eba6f89 Compare October 18, 2022 19:38
@amotl amotl force-pushed the amo/timezone-aware-datetimes branch 2 times, most recently from 98630d0 to 48d8bd7 Compare November 21, 2022 16:27
@amotl amotl force-pushed the amo/timezone-aware-datetimes branch from 48d8bd7 to 77dc5df Compare December 2, 2022 19:46
@amotl amotl force-pushed the amo/timezone-aware-datetimes branch from 77dc5df to 8d8a6d1 Compare December 8, 2022 15:36
Base automatically changed from amo/timezone-aware-datetimes to master December 8, 2022 15:52
@amotl amotl changed the title [STACK] Refactor certain doctests towards either reStructuredText documentation or Python tests Refactor certain doctests towards either reStructuredText documentation or Python tests Dec 8, 2022
The referenced doctests are now becoming first citizens of the published
documentation, within a `by-example` section. To make sure they are
always valid, they are still evaluated as doctests.

`doctests/mocking.txt` has been translated into a pure-Python test case.
- doctests/sqlalchemy.rst
- sqlalchemy/doctests/{dialect,itests,reflection}.txt
This is a preparation for:

    SQLALCHEMY_WARN_20=1 ./bin/test -vvv -t SqlAlchemy
@@ -0,0 +1,122 @@
=====================================================
SQLAlchemy: Database schema inspection and reflection
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is anything of this section CrateDB specific? Looks like it's mostly explaining SQLAlchemy, in which case we could refer to the upstream docs instead and test the functionality in python tests

Copy link
Member Author

@amotl amotl Dec 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi. While the SQLAlchemy API methods per se are not specific to CrateDB, I found this section worth to keep, in order to display different kinds of inspection and reflection methods of SQLAlchemy together with their slightly CrateDB-specific outcomes, also because they are not mentioned elsewhere in our documentation (modulo has_table), and also because the corresponding spots are a bit obfuscated within the SQLAlchemy documentation.

In other words, while bringing those snippets together from different parts of our documentation, I have made an extra effort to discriminate between the different kinds of introspection/reflection features offered by SQLAlchemy.

  • SQLAlchemy inspector
  • Schema-supported reflection
  • CrateDialect-specific reflection methods

Some examples from the upstream documentation at runtime inspection API, SQLAlchemy inspector, and Schema-supported reflection, have been paired with CrateDB-specific responses.

The result is a reasonably concise overview about them

>>> inspector.get_schema_names()
['blob', 'doc', 'information_schema', 'pg_catalog', 'sys']

>>> inspector.default_schema_name
'doc'

>>> dialect.has_schema(connection, 'doc')
True

Other than this, some parts of the documentation section within SQLAlchemy: Create, retrieve, update, and delete are also just vanilla SQLAlchemy operations, but I think the trio of getting-started.rst, crud.rst, and inspection-reflection.rst still make sense in the context of the CrateDB driver, both as documentation, as well as a doctest scenario. Now that they have been refactored, I think they are in a better shape than before.

docs/by-example/connection.rst Outdated Show resolved Hide resolved
docs/by-example/cursor.rst Outdated Show resolved Hide resolved
docs/by-example/index.rst Outdated Show resolved Hide resolved
docs/by-example/index.rst Outdated Show resolved Hide resolved
docs/by-example/sqlalchemy/crud.rst Outdated Show resolved Hide resolved
docs/by-example/sqlalchemy/crud.rst Outdated Show resolved Hide resolved
Comment on lines 5 to 8
This section of the documentation outlines how to use CrateDB's SQLAlchemy
integration. It demonstrates both basic database operations (insert, select,
delete), creating and dropping tables, running queries with aggregations,
as well as the use of complex and geospatial data types.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds similar to the crud document. Would it make sense to merge the two into one? Or instead clarify the difference and link from one to the other.

Copy link
Member Author

@amotl amotl Dec 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your attention. You are right. I've tried to improve this now with 5dc0e04.

The previous version was a bit of a potpourri of different topics. From that mix, two new documents have been created. One is about "advanced querying", including exercises about aggregations and full-text capabilities, and the other one exercises operations on the CrateDB container and geospatial special types concisely, now also consistently using the Object and ObjectArray type aliases, instead of the funny-sounding Craty type.

I will be happy about any kinds of suggestions on improving the wording/tone, like your previous excellent patches.

docs/by-example/sqlalchemy/inspection-reflection.rst Outdated Show resolved Hide resolved
docs/by-example/sqlalchemy/inspection-reflection.rst Outdated Show resolved Hide resolved
docs/by-example/sqlalchemy/inspection-reflection.rst Outdated Show resolved Hide resolved
@amotl amotl force-pushed the amo/doctests-refactoring branch 2 times, most recently from 55cac25 to 6090d53 Compare December 20, 2022 21:38
The previous version was a bit of a potpourri of different topics. From
that mix, two new documents have been created. One is about "advanced 
querying", including aggregations and full-text capabilities, and the
other one exercises operations on the CrateDB container and geospatial
special types concisely.
Copy link
Member

@mfussenegger mfussenegger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some more suggestions. The new structure looks much better 👍

docs/by-example/sqlalchemy/advanced-querying.rst Outdated Show resolved Hide resolved
Comment on lines 115 to 116
CrateDB currently does not support all variants as it can not handle the
sub-queries yet.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still the case?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. I also wanted to highlight this note while reading it, but missed to do so. Which suggestion do you have?
a) Investigate the matter.
b) Just remove this sentence.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a) + create an issue in the crate repo to support it if there are variants which it can't handle

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will do. However, to move on with this, and other subsequent patches, I've deferred it to GH-489. I hope this is fine.

Comment on lines 108 to 109
Aggregates: Counting and grouping
=================================
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe move this elsewhere - the previous section is "Introduction to fulltext indexes" and the next section is "Fulltext search with MATCH predicate" - having an intermediate aggregation/count/group-by breaks the flow

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree and will try to find a different spot.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The section has been moved one slot to the bottom with fe90300.

docs/by-example/sqlalchemy/advanced-querying.rst Outdated Show resolved Hide resolved
docs/by-example/sqlalchemy/advanced-querying.rst Outdated Show resolved Hide resolved
docs/by-example/sqlalchemy/advanced-querying.rst Outdated Show resolved Hide resolved
docs/by-example/sqlalchemy/advanced-querying.rst Outdated Show resolved Hide resolved
Co-authored-by: Mathias Fußenegger <[email protected]>
The "Aggregates: Counting and grouping" section breaks the reading flow,
in between the full-text topic, which should not be separated.
Copy link
Member Author

@amotl amotl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for your excellent contributions to this patch, @hammerhead and @mfussenegger. Merging it now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants